Update tests to stop relying on DiagnosticAnalyzerTestBase #2988

Evangelink · 2019-10-30T12:21:25Z

Motivation

In one of the PR review, @sharwell said that inheriting from DiagnosticAnalyzerTestBase was the old (obsolete?) way of doing tests. Some preparation for the migration was already done by adding the static usings for C# and VB.NET.

This PR tries to replace most of the old usages with the new recommended.

What's left

There is still some work left but I am not sure whether this shall be done within this PR or into some other. Also, I need some input from you to move forward on some points.

Non migrated tests

Some tests are not migrated because:

usage of the scope notation doesn't seem to be working properly. I didn't spend time to do investigate what was the issue. I will try to do so later on.
some tests explicitly force the parse option to null, although I think I might have found a way to do the same thing with the new syntax, I am not sure about it.
~~something wasn't working properly when multiple "files" were given~~
I missed some test classes

AD0001

The following tests from ReviewUnusedParametersTests.cs:

public async Task DiagnosticForUnusedLocalFunctionParameters_01()
public async Task DiagnosticForUnusedLocalFunctionParameters_02()
public async Task NoDiagnosticUsedLocalFunctionParameters()

are failing with the following error: error AD0001: Analyzer 'Microsoft.CodeQuality.Analyzers.Maintainability.ReviewUnusedParametersAnalyzer' threw an exception of type 'System.ArgumentException' with message 'The key already existed in the dictionary.'.

~~I will create a ticket to handle this issue but I am not sure whether to revert the failing tests to the old way or to skip them for the time being. WDYT?~~

Miscellaneous

In some tests the callers always provide the same rule, it could be move directly to the get diagnostic result method.
Some tests need to be updated to use the descriptor instead of the ID.
I am not sure whether tests shall be using new DiagnosticResult() or VerifyCS.Diagnostic()

Hope this PR is going the right direction and helps you!

sharwell

It looks like your editor is misconfigured to make whitespace changes unrelated to the rest of the pull request. I stopped the review after a few files due to the high level of noise caused by this. If the editor was Visual Studio and there is no extension in use that was causing these changes, we will definitely want to get a bug filed to force it to stop this.

...soft.CodeAnalysis.Analyzers/UnitTests/MetaAnalyzers/AddLanguageSupportToAnalyzerRuleTests.cs

...odeAnalysis.Analyzers/UnitTests/MetaAnalyzers/MissingDiagnosticAnalyzerAttributeRuleTests.cs

...icrosoft.CodeAnalysis.Analyzers/UnitTests/FixAnalyzers/FixerWithFixAllAnalyzerTests.Fixer.cs

src/Test.Utilities/CSharpCodeFixVerifier`2.cs

...osoft.NetCore.Analyzers/UnitTests/Tasks/DoNotCreateTasksWithoutPassingATaskSchedulerTests.cs

Evangelink · 2019-10-30T15:14:30Z

@sharwell Sorry for the time waste. I did all the changes without thinking to disable some of the extensions. I am used to rely on them for git development as they help avoiding those kind of space/tab or trailing space "issue".

If that's ok for you I will do a revert, un-stage all spaces and force-push right after.

sharwell · 2019-10-31T14:53:17Z

@Evangelink If you are OK with it, I should be able to selectively revert those lines as long as you are OK with me force-pushing to your verifycs-test branch. Let me know if you would prefer I do that.

If you are comfortable with it, please feel free to force-push to this PR to make the change.

Evangelink · 2019-10-31T15:12:29Z

@sharwell Of course I am fine with you doing some changes as needed in this PR.

There were too many conflict with the master branch (because of the culture for the string format) + all those incorrect spaces/tabs issues so I went for a rebase + re-apply of all changes 1 by 1. Hopefully everything shall be alright for you to review.

src/Microsoft.CodeAnalysis.Analyzers/UnitTests/FixAnalyzers/FixerWithFixAllAnalyzerTests.cs

...ality.Analyzers/UnitTests/ApiDesignGuidelines/AbstractTypesShouldNotHaveConstructorsTests.cs

src/Microsoft.CodeQuality.Analyzers/UnitTests/ApiDesignGuidelines/AvoidEmptyInterfacesTests.cs

...lity.Analyzers/UnitTests/ApiDesignGuidelines/DoNotDeclareStaticMembersOnGenericTypesTests.cs

...t.CodeQuality.Analyzers/UnitTests/Maintainability/AvoidUninstantiatedInternalClassesTests.cs

src/Microsoft.CodeQuality.Analyzers/UnitTests/Maintainability/DoNotIgnoreMethodResultsTests.cs

...t.CodeQuality.Analyzers/UnitTests/QualityGuidelines/ValidateArgumentsOfPublicMethodsTests.cs

sharwell · 2019-11-01T14:44:29Z

@Evangelink I was able to complete the review. Please avoid rebasing at this point so the updates are straightforward to review. 👍

Evangelink · 2019-11-01T16:45:49Z

It would be nice to have some info about the failing tests in the CI, they are all green locally so it's a bit hard to understand what's wrong.

sharwell · 2019-11-01T16:47:38Z

Does this show for you? https://dev.azure.com/dnceng/public/_build/results?buildId=411650&view=ms.vss-test-web.build-test-results-tab

It should appear from this link:

sharwell · 2019-11-01T16:48:47Z

VisualBasic_ProtectedVariable_PublicContainingType

Microsoft.CodeAnalysis.Testing.Verifiers.EqualWithMessageException : Context: Diagnostics of test state
Expected diagnostic message to be \"No declarar campos de instancia visibles\" was \"Do not declare visible instance fields\"

Diagnostic:
 // Test0.vb(3,15): warning CA1051: Do not declare visible instance fields
GetBasicResultAt(3, 15, DoNotDeclareVisibleInstanceFieldsAnalyzer.CA1051)


Assert.Equal() Failure
Expected: No declarar campos de instancia visibles
Actual: Do not declare visible instance fields

   at Microsoft.CodeAnalysis.Testing.Verifiers.XUnitVerifier.Equal[T](T expected, T actual, String message) in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Testing.Verifiers.XUnit/XUnitVerifier.cs:line 48
   at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.VerifyDiagnosticResults(IEnumerable`1 actualResults, ImmutableArray`1 analyzers, DiagnosticResult[] expectedResults, IVerifier verifier) in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs:line 313
   at Microsoft.CodeAnalysis.Testing.AnalyzerTest`1.<VerifyDiagnosticsAsync>d__51.MoveNext() in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.Analyzer.Testing/AnalyzerTest`1.cs:line 194
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable.ConfiguredTaskAwaiter.GetResult()
   at Microsoft.CodeAnalysis.Testing.CodeFixTest`1.<RunAsync>d__38.MoveNext() in /_/src/Microsoft.CodeAnalysis.Testing/Microsoft.CodeAnalysis.CodeFix.Testing/CodeFixTest`1.cs:line 189
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
   at Test.Utilities.VisualBasicCodeFixVerifier`2.<VerifyAnalyzerAsync>d__5.MoveNext() in /_/src/Test.Utilities/VisualBasicCodeFixVerifier`2.cs:line 38
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
   at Test.Utilities.VisualBasicCodeFixVerifier`2.<VerifyAnalyzerAsync>d__4.MoveNext() in /_/src/Test.Utilities/VisualBasicCodeFixVerifier`2.cs:line 27
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
   at Microsoft.CodeQuality.Analyzers.ApiDesignGuidelines.UnitTests.DoNotDeclareVisibleInstanceFieldsTests.<VisualBasic_ProtectedVariable_PublicContainingType>d__13.MoveNext() in /_/src/Microsoft.CodeQuality.Analyzers/UnitTests/ApiDesignGuidelines/DoNotDeclareVisibleInstanceFieldsTests.cs:line 156
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)

...odeQuality.Analyzers/UnitTests/ApiDesignGuidelines/DoNotDeclareVisibleInstanceFieldsTests.cs

sharwell · 2019-11-01T21:14:40Z

@Evangelink apparently just one test class is causing problems. The two specific changes I mentioned just now should resolve the test failures, and then we can merge this.

Evangelink · 2019-11-04T10:44:49Z

@sharwell I was actually looking at the logs and couldn't find anything except the error lines (without any detail on the failing tests), I totally forgot to look at the Tests tab. Thanks!

sharwell requested changes Oct 30, 2019

View reviewed changes

Evangelink added 3 commits October 31, 2019 14:36

Stop using DiagnosticAnalyzerTestBase

f34b5cc

Fix rebase incorrect merge

de31877

Use the "long form" test instead of a new helper overload

1bba41a

Evangelink force-pushed the verifycs-test branch from 4d59ac7 to 1bba41a Compare October 31, 2019 14:31

Evangelink requested a review from sharwell October 31, 2019 14:48

sharwell reviewed Nov 1, 2019

View reviewed changes

Evangelink mentioned this pull request Nov 1, 2019

Review uses of CompilerDiagnostics.None #2995

Closed

Evangelink added 2 commits November 1, 2019 17:15

Address review comments

ed5349f

Skip tests failing with AD0001 and link them to an issue

8092261

Evangelink requested a review from sharwell November 1, 2019 16:23

Evangelink mentioned this pull request Nov 1, 2019

Use package references instead of local fake definitions #2997

Closed

sharwell approved these changes Nov 1, 2019

View reviewed changes

sharwell reviewed Nov 1, 2019

View reviewed changes

...odeQuality.Analyzers/UnitTests/ApiDesignGuidelines/DoNotDeclareVisibleInstanceFieldsTests.cs Outdated Show resolved Hide resolved

...odeQuality.Analyzers/UnitTests/ApiDesignGuidelines/DoNotDeclareVisibleInstanceFieldsTests.cs Outdated Show resolved Hide resolved

Simplify DiagnosticResult creation

fa917f1

sharwell merged commit af47226 into dotnet:master Nov 1, 2019

Evangelink deleted the verifycs-test branch November 4, 2019 09:49

Evangelink mentioned this pull request Nov 4, 2019

Couple of small changes post migration to new test framework #3006

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update tests to stop relying on DiagnosticAnalyzerTestBase #2988

Update tests to stop relying on DiagnosticAnalyzerTestBase #2988

Evangelink commented Oct 30, 2019 •

edited

Loading

sharwell left a comment

Evangelink commented Oct 30, 2019

sharwell commented Oct 31, 2019 •

edited

Loading

Evangelink commented Oct 31, 2019

sharwell commented Nov 1, 2019

Evangelink commented Nov 1, 2019

sharwell commented Nov 1, 2019 •

edited

Loading

sharwell commented Nov 1, 2019 •

edited

Loading

sharwell commented Nov 1, 2019

Evangelink commented Nov 4, 2019

Update tests to stop relying on DiagnosticAnalyzerTestBase #2988

Update tests to stop relying on DiagnosticAnalyzerTestBase #2988

Conversation

Evangelink commented Oct 30, 2019 • edited Loading

Motivation

What's left

Non migrated tests

AD0001

Miscellaneous

sharwell left a comment

Choose a reason for hiding this comment

Evangelink commented Oct 30, 2019

sharwell commented Oct 31, 2019 • edited Loading

Evangelink commented Oct 31, 2019

sharwell commented Nov 1, 2019

Evangelink commented Nov 1, 2019

sharwell commented Nov 1, 2019 • edited Loading

sharwell commented Nov 1, 2019 • edited Loading

sharwell commented Nov 1, 2019

Evangelink commented Nov 4, 2019

Evangelink commented Oct 30, 2019 •

edited

Loading

sharwell commented Oct 31, 2019 •

edited

Loading

sharwell commented Nov 1, 2019 •

edited

Loading

sharwell commented Nov 1, 2019 •

edited

Loading